Fix encapsulated pixeldata handling #103
Conversation
… specific e.g. in case of CT it can be 12,13,14,15,16
…ally not read, undefined length was assumed use more intuitive offset calculation: previous offset + tag and length size + current length
Hi, thank you for your response. |
|
OK, let's tag this as a draft for now. |
fix indexing problem when creating offsets table fix frame size calculation: bits allocated was not considered
|
I think I solved the problems. |
|
@jcupitt What's your opinion about this PR? The other PR (the one before this) is updated. If that is merged, then I'll rebase and update my PR. |
|
Hi @weanti, sorry, I was on holiday and then got distracted by other projects. I'll look this over again now. |
|
... I saw one final tiny issue in #102, when that's resolved I'll look at this more closely. |
Thank you for the feedback. |
|
Resolved conflicts. |
|
I'll read this tomorrow. Thanks for the update! |
|
I downloaded the zip file, these are useful samples! What's the licence? Could we add them to the test suite? |
|
|
Great! Still to do:
|
@dclunie can you confirm what is the license for the images available from the NEMA FTP server? @michaelonken @jriesmeier how about the images shared in https://www.dcmtk.org/download/images/ ? |
|
There is no specified license for the CAR97, NEMA97, or WG04 images - we had intended all of these to be publicly usable without restriction (we gave away the CDs at meetings, and shared the images online by anonymous ftp), but never defined a license. |
|
David said it all ☝️ [Edit: The folder ddsm/ has its own README. The collection of images stems from the University of South Florida and has originally been provided in non-DICOM format. We converted it (I don't remember the exact context) and offered to host these images on the OFFIS servers (which they were fine with). I found a copy of the now-offline original website in the Internet archive. The images have been part of a research grant; maybe you find more information if you dig through the site.] |
I'm working on tests. Will take some time due to limited availability. |
|
Added some tests. These are rather functional test, because the implementation that handles encapsulated pixel data is not on the public API. |
|
@jcupitt Shall we proceed with this PR? |
|
Sorry @weanti I got distracted again. I'll do a final review now. |
jcupitt
left a comment
jcupitt
left a comment
There was a problem hiding this comment.
Sorry :( one more thing.
| if (!read_tag(&state, &tag, &position) || | ||
| !read_uint32(&state, &length, &position)) { | ||
| return false; | ||
| // each frame may consist of several fragments, so we need to scan each fragment to find the next frame |
There was a problem hiding this comment.
I don't think you need this complicated loop. I think (I think! I hope!) all you need to do is scan num_frames items and collect the offsets. This will collect the right number of frames whether or not frames are split into many items.
How about changing this back to:
turn false;
}
} else {
// the BOT is missing, we must scan pixeldata to find the position of
// each frame
dcm_log_info("building Offset Table from Pixel Data");
// 0 in the BOT is the offset to the start of frame 1, ie. here
*first_frame_offset = position;
position = 0;
for (int i = 0; i < num_frames; i++) {
if (!read_tag(&state, &tag, &position) ||
!read_uint32(&state, &length, &position)) {
return false;
}
if (tag == TAG_SQ_DELIM) {
dcm_error_set(error, DCM_ERROR_CODE_PARSE,
"reading BasicOffsetTable failed",
"too few frames in PixelData");
return false;
}
if (tag != TAG_ITEM) {
dcm_error_set(error, DCM_ERROR_CODE_PARSE,
"building BasicOffsetTable failed",
"frame Item #%d has wrong tag '%08x'",
i + 1,
tag);
return false;
}
// step back to the start of the item for this frame
offsets[i] = position - 8;
// and seek forward over the value
if (!dcm_seekcur(&state, length, &position)) {
return false;
}
}
}
return true;
}ie. just removing the check for the end of sequence tag.
I suppose we could check for end-of-sequence in the num_frames > 1 case, but I don't know if it'd add much.
| fragment_length = 0; | ||
| char* fragment = value; | ||
| position = 0; | ||
| while(position < frame_end_offset) { |
|
I'm still worried by the BOT builder, I think you don't need the new loop you made. How about just reading One optimisation we could do would be to change this around to scan all items rather than scan all frames. We could record total frame length somewhere, then The new code isn't following the libdicom layout style exactly, but I can fix that after merge if you like (or do change it yourself, of course). Otherwise this looks good! |
| return NULL; | ||
| } | ||
| dcm_seekcur(&state, fragment_length, &position); | ||
| *length += fragment_length; |
There was a problem hiding this comment.
I suppose there's a potential uint32 overflow here. length should ideally be a uint64, though I don't suppose it matters much.
5 participants
Fix constructing frame offsets for encapsulated pixel data.
Fix reading frames from encapsulated pixel data.
Remove restrictions for bits stored value, because it is not valid in all cases e.g. CT.